Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TUF Python Client Example/Tutorial #1675

Conversation

kairoaraujo
Copy link
Collaborator

It is a simple example of TUF ngclient implementation.

This example contains a README.rst that is a tutorial/how-to-use
this simple client using static test data from TUF repository.

The code aims to be straightforward implementation, using basic
concepts from Python and Command Line Interface.

This is part of #1518

Signed-off-by: Kairo de Araujo [email protected]

Please fill in the fields below to submit a pull request. The more information
that is provided, the better.

Description of the changes being introduced by the pull request:

Please verify and check that the pull request fulfills the following
requirements
:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

@kairoaraujo
Copy link
Collaborator Author

I am still tagging it as WIP to improve in general.
The code uses basic concepts about Python. I avoided OO models and implemented the minimum number of functions to be more sequential and easier to understand. Is it a good approach?

The client_example is a subfolder in examples, and the README.rst can be visible as the tutorial for it, any better suggestion?

Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, thanks. Some comments (some of these could be left for future work):

  • I think argparse is probably a good choice: it's verbose but part of the base library and will allow multiple subcommands without making a mess
  • init should be implicit, I think (as in it should always happen)? it's the correct example for real clients and as long as the printed output is valid it should never be a problem either (also if init is implicit, then it can probably be combined with tuf_updater)
  • I would not use "[INFO]" and "[ERROR]" like this: to me it implies you are using logging and that I can decrease/increase verbosity but I can't, it's just print()... maybe you should be using logging (and set default level to INFO or something)?
  • linting: we want this code linted (with the same config that ngclient uses)
  • docs: I think in the end the documentation should get included in the published docs (probably under a new top level "examples" title)... this does not have to happen right now but I think we should delay reviewing and polishing the doc until we see the published form. My gut feeling is that we should not include details like how to clone the repo or where to find the example code etc: these are things that change over time and then someone would need to update them. We have a "instructions for contributors" document, let's link to that if we really have to but let's not repeat ourselves.

@coveralls
Copy link

coveralls commented Nov 15, 2021

Pull Request Test Coverage Report for Build 1536678421

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 97.581%

Totals Coverage Status
Change from base Build 1534603791: 0.0%
Covered Lines: 4032
Relevant Lines: 4116

💛 - Coveralls

Copy link
Member

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool stuff! I suggest we start running pylint/black/isort over code in the new examples directory too. Can you add the directory to the lint directive in tox.ini?

Also, I think the printed info text could be slightly improved (I say this as a non-native English speaker). :)

examples/client_example/client_example.py Outdated Show resolved Hide resolved
@lukpueh
Copy link
Member

lukpueh commented Nov 15, 2021

I have to say that English is the hard part for me here. 😅

Maybe we can ask our technical writer and chief of words @jhdalek55 for help with that.

@kairoaraujo
Copy link
Collaborator Author

nice, thanks. Some comments (some of these could be left for future work):

  • I think argparse is probably a good choice: it's verbose but part of the base library and will allow multiple subcommands without making a mess

Yes, there are more fancy libraries, but I think argparse will do the work without extra dependencies.

  • init should be implicit, I think (as in it should always happen)? it's the correct example for real clients and as long as the printed output is valid it should never be a problem either (also if init is implicit, then it can probably be combined with tuf_updater)

Yes, indeed. I was in doubt about which direction to choose.

  • I would not use "[INFO]" and "[ERROR]" like this: to me it implies you are using logging and that I can decrease/increase verbosity but I can't, it's just print()... maybe you should be using logging (and set default level to INFO or something)?

Actually, I started with logging, but I tried to make it less complex in the sense most of the basic tutorials try to use "print".
I'm open to both.

  • linting: we want this code linted (with the same config that ngclient uses)

👍

  • docs: I think in the end the documentation should get included in the published docs (probably under a new top level "examples" title)... this does not have to happen right now but I think we should delay reviewing and polishing the doc until we see the published form. My gut feeling is that we should not include details like how to clone the repo or where to find the example code etc: these are things that change over time and then someone would need to update them. We have a "instructions for contributors" document, let's link to that if we really have to but let's not repeat ourselves.

I understand that we do not want to repeat ourselves, making it more maintainable, and I agree.
I think the challenge here is to give to who land to the example tutorial a non-segregated experience. I mean, something that you can follow in a single experience and not switch across different docs.

@jhdalek55
Copy link
Contributor

@jhdalek55 ...I'm happy to work on any English language issues. Just let me know what needs to be reviewed and when. I generally prefer to hold off on reviews until the technical content is more or less confirmed.

@jku
Copy link
Member

jku commented Nov 16, 2021

Actually, I started with logging, but I tried to make it less complex in the sense most of the basic tutorials try to use "print".
I'm open to both.

Same, either way works for me.

On the question of "is this good for a tutorial/example?", I think the goals should be

  • example that works out of the box
  • example code that shows how to write good code that uses python-tuf
  • tutorial material

in this order: my opinion is that good example code is more important than a tutorial that literally anyone could follow.

@kairoaraujo
Copy link
Collaborator Author

The last commit addresses more general aspects such as lint, README format, etc.
I will work on some designs improvement mentioned by @jku , so still WIP. 👨🏽‍💻

@kairoaraujo
Copy link
Collaborator Author

Some questions:

@lukpueh
Copy link
Member

lukpueh commented Nov 22, 2021

Let me finalize my example in that PR and gather some more feedback. I'm still unsure, if a markdown wouldn't be more appropriate, in terms of making the accompanying text more readable. IMO the question boils down to readability vs. ease of testing. (see #808 (comment) and in-toto/demo#7 for related discussion about markdown doc testing)

@jku
Copy link
Member

jku commented Nov 25, 2021

I think your code and 1685 are different for two reasons:

  1. this code can be, or can become, a "real" example application. The metadata API tutorial is a stopgap solution until we have a repository library
  2. the client side is "well defined": all clients will operate in much the same way so it is possible for us to define the "canonical use pattern". Repository code on the other hand might be wildly different (think CLI apps vs Warehouse)

So I don't think there's a need for these two solutions to look similar.

  • Should we add to the example code more TUF exceptions handling

For the client app my thinking has been that it should need to handle RepositoryError only (although I see you had to handle some requests issues as well -- we can consider those after this merges): RepositoryError means the update cannot continue because of an issue with TUF metadata: there could be many specific reasons but the client code cannot possibly deal with any of them.

Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow, ended up with a lot of comments -- if this is still WIP don't mind me

examples/client_example/client_example.py Outdated Show resolved Hide resolved
examples/client_example/client_example.py Outdated Show resolved Hide resolved
examples/client_example/client_example.py Outdated Show resolved Hide resolved
examples/client_example/client_example.py Outdated Show resolved Hide resolved
examples/client_example/client_example.py Outdated Show resolved Hide resolved
path = updater.find_cached_target(info)
print(f"Cached target {target} verified")
if path:
print(f"Target is already available in {DOWNLOAD_DIR}/{info.path}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not always correct -- info.path is the url path which may or may not match the filename on disk. path contains the filesystem path

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will review what I did, but insights are welcome.

examples/client_example/client_example.py Outdated Show resolved Hide resolved
examples/client_example/client_example.py Outdated Show resolved Hide resolved
examples/client_example/client_example.py Outdated Show resolved Hide resolved
examples/client_example/client_example.py Outdated Show resolved Hide resolved
@jku
Copy link
Member

jku commented Nov 25, 2021

This should probably be a future PR: we want the example tested by CI (should file a new issue for that)

For inspiration, I have some (quite bad) tests for a CLI app in https://github.com/vmware-labs/repository-editor-for-tuf/blob/main/tests/test_cli.py -- I think just a very simple test is fine: run a few commands, check if output contains correct keywords, check that files exist where they should

@kairoaraujo kairoaraujo changed the title WIP: TUF Python Client Example/Tutorial TUF Python Client Example/Tutorial Nov 26, 2021
@kairoaraujo
Copy link
Collaborator Author

I am still looking if download() could be more "elegant." More about the exception handling.

Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the overall structure is good: client_args construction could be in its own function but so far it's so simple that I don't mind the current way.

Can you enable mypy for this? (currently still happens in setup.cfg I think)

examples/client_example/README.md Outdated Show resolved Hide resolved
examples/client_example/README.md Outdated Show resolved Hide resolved
examples/client_example/README.md Outdated Show resolved Hide resolved
examples/client_example/README.md Outdated Show resolved Hide resolved
examples/client_example/client_example.py Outdated Show resolved Hide resolved
examples/client_example/client_example.py Outdated Show resolved Hide resolved
examples/client_example/client_example.py Outdated Show resolved Hide resolved
examples/client_example/client_example.py Outdated Show resolved Hide resolved
examples/client_example/client_example.py Outdated Show resolved Hide resolved
examples/client_example/client_example.py Outdated Show resolved Hide resolved
Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good: we'll definitely want to keep improving but this looks like a fine first step. The commit history could be squashed as it's a little random but since it's new code I don't really mind either way.

Anyone else want to have opinions here (@joshuagl, @sechkova)?

Potential future work:

  • Consider if we want the README published on RTD, modify it as required if we do
  • Add a simple test for the example
  • support other server locations than hard-coded localhost:8000
  • support boostrapping root.json for other servers (like the repository generated by the repository example) somehow
  • document the application decisions better (as an example "why is is local metadata stored in $HOME/.local/share/", "why not download first root.json over http?"). I didn't do this in the review as I think it's a tricky balance: too much detail means no-one will read it...
  • start thinking about how to use examples to show that TUF actually works, and prevents attacks. This might be more related to repository examples but ...

@jku
Copy link
Member

jku commented Dec 1, 2021

I didn't actually run this before mysefl but now I did: I seem to get debug level output if i don't provide a verbosity level? This clearly seems like a bug?

lukpueh added a commit to lukpueh/tuf that referenced this pull request Dec 2, 2021
Add 1.0.0 announcment document and point to it in main README.

TODO:
- Commit message
- PR (blocks on theupdateframework#1693, theupdateframework#1675, maybe theupdateframework#1700)
@lukpueh lukpueh mentioned this pull request Dec 3, 2021
3 tasks
Kairo de Araujo added 6 commits December 3, 2021 19:57
It is a simple example of TUF ngclient implementation.

This example contains a README.rst that is a tutorial/how-to-use
this simple client using static test data from TUF repository.

The code aims to be straightforward implementation, using basic
concepts from Python and Command Line Interface.

This is part of theupdateframework#1518

Signed-off-by: Kairo de Araujo <[email protected]>
- Added the lint to examples
- README format moved from Restructuredtext to Markdown
- Removed the [INFO] and [ERROR] from output, to avoid confundint with
  logging structure

Signed-off-by: Kairo de Araujo <[email protected]>
Using ``print`` for high-level client output
Option to see ``tuf.ngclient`` logging output

Update the README.

Signed-off-by: Kairo de Araujo <[email protected]>
To highlight TUF ngclient exceptions.

Signed-off-by: Kairo de Araujo <[email protected]>
This commit adds many comments from the WIP stage

Signed-off-by: Kairo de Araujo <[email protected]>
To simple exemplify, the updater calls are added to a single block
that handles the exceptions

Signed-off-by: Kairo de Araujo <[email protected]>
Kairo de Araujo added 2 commits December 3, 2021 19:59
- Simplify README and better keywords
- Fix the verbosity
- Better docstrings
- Client flow for init and main are clear

Signed-off-by: Kairo de Araujo <[email protected]>
without -v is ERROR logging, -v  WARNINGS logging, -vv INFO logging
and -vvv (+) DEBUG logging.

Signed-off-by: Kairo de Araujo <[email protected]>
@kairoaraujo kairoaraujo force-pushed the issue#1518/python-client-example-tutorial branch from 5082b16 to 1344410 Compare December 3, 2021 19:00
@jku
Copy link
Member

jku commented Dec 7, 2021

Merging this.

Please file a new issue for adding a test for this example.

Maybe a new issue for the RTD documention inclusion as well? I'm not 100% sure we should include the example there but maybe someone should take a stab and see if it works well and whether it needs any changes.

@jku jku merged commit 2db0cd6 into theupdateframework:develop Dec 7, 2021
@lukpueh lukpueh mentioned this pull request Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants